-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import/Export command #232
Import/Export command #232
Conversation
Hey Wen Jun, some thoughts & suggestions:
Looking forward to test out how your feature works! |
src/main/java/seedu/address/logic/commands/ExportEventXmlCommand.java
Outdated
Show resolved
Hide resolved
|
||
// Setting up file path | ||
File output = new File(System.getProperty("user.home") + "/Desktop/" | ||
+ event.getName().toString() + ".xml"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to include the event ID here as well, such that the export file name takes the format <EVENTNAME>_<EVENTID>.xml
DOMSource source = new DOMSource(doc); | ||
|
||
// Setting up file path | ||
File output = new File(System.getProperty("user.home") + "/Desktop/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would personally suggest extracting System.getProperty("user.home") + "/Desktop/"
out to a field named SAVE_PATH
, so as to be able to plug it in multiple places without worrying about errors or having to edit each usage location separately if you wish to update the save path.
src/main/java/seedu/address/logic/commands/ImportVolunteerCsvCommand.java
Show resolved
Hide resolved
src/main/java/seedu/address/logic/commands/ImportVolunteerCsvCommand.java
Outdated
Show resolved
Hide resolved
|
||
try { | ||
createEventXml(model, selectedEvent); | ||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be useful to find out what type(s) of Exception may result from running this method call, and handling them more specifically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used it as a blanket statement for now because of possible file creation exceptions when handling the xml transformer and writing throws out to an error at creating the file. I will update the individual exceptions accordingly although they have the same handler
*/ | ||
private void createVolunteerCsv(ObservableList<Volunteer> list, ArrayList<Index> index) throws Exception { | ||
// Setting up file path | ||
File output = new File(System.getProperty("user.home") + "/Desktop/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desktop might not be a good choice. Better to use relative path
|
||
if (!model.hasVolunteer(volunteer)) { | ||
model.addVolunteer(volunteer); | ||
model.commitAddressBook(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the commit here? Placing it here means:
- When user uses the undo command, it only removes the add of the last volunteer added.
It is better to put the model.commitAddressBook() method outside. -> When ALL the volunteers have been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted and will update accordingly
public ExportVolunteerXmlCommand parse(String userInput) throws ParseException { | ||
requireNonNull(userInput); | ||
|
||
Index index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this command only take in 1 volunteer to export in the current state?
Maybe should have a good use case for this command before updating, XML to XML doesn't seem to be a feature that makes sense from the end user perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current state takes in only 1 volunteer, but it calls out all the events this volunteer has participated in and logs the information about these events as well into the xml output. Main focus of this feature is to list all the events a volunteer has participated as an export.
|
||
try { | ||
createVolunteerCsv(model.getFilteredVolunteerList(), index); | ||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like Sharan mentioned, should handle specific exceptions instead of using a catch all Exception
// Setting up writer & stringbuilder for appending | ||
PrintWriter pw = new PrintWriter(output); | ||
StringBuilder sb = new StringBuilder(); | ||
String csvSplit = ","; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to test this with fields that could contain commas, like Address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assumption on the csv entries dont have characters which messes with the tuple splits, like comma (,)and bracer ("").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not work under this assumption. Users should be allowed to enter commas for addresses. Try to find a way to clean the input. Maybe this link would be helpful.
…nd.java Co-Authored-By: Scrubbius <36285531+Scrubbius@users.noreply.github.com>
…ommand.java Co-Authored-By: Scrubbius <36285531+Scrubbius@users.noreply.github.com>
…ommand.java Co-Authored-By: Scrubbius <36285531+Scrubbius@users.noreply.github.com>
ExportEventXml now has event ID on part of the filename File directory created for the specific exports as well with the desktop as a fall-back
Not going to split the export command into -csv and -xml as part of the parameters since the xml command takes in only 1 volunteer at a time while the csv command merges all into one file. Maybe the xml variant can be updated in the future to take in more than one volunteer index and produce the files accordingly but for now it is an auxiliary command for individual information exporting. |
…to ImportCommands Conflicts resolved
…mmas Update exportvolunteercsv to have compatibility with the import format
import commands added on top of the export ones.